Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: big refactor to add waku component in libwaku instead of only waku node #2658

Merged
merged 4 commits into from
May 3, 2024

Conversation

Ivansete-status
Copy link
Collaborator

Description

With this PR we:

  • rename the app.nim module to waku.nim
  • rename App type to Waku type. We start considering Waku as the type that contains WakuNode, discovery elements, and will contain an instance of PeerManager in the future. Waku type represents Waku ;P (very original name ;P)
  • refactor libwaku code so that we start dealing with the Waku object from within libwaku and therefore, start having a similar behaviour as the wakunode2 app

Issue

migrate DiscV5 and DNS Discovery from app.nim to waku_node.nim - #2452

Copy link

github-actions bot commented May 2, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2658-rln-v1

Built from 92f68d9

Copy link

github-actions bot commented May 2, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2658-rln-v2

Built from 92f68d9

Copy link
Contributor

@rymnc rymnc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small comment, there are tools/applications which may use the App* export, can we rename them too?

@Ivansete-status Ivansete-status marked this pull request as ready for review May 2, 2024 13:15
Copy link
Contributor

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thank you!

waku/node/waku_node.nim Show resolved Hide resolved
waku/waku_peer_exchange/protocol.nim Show resolved Hide resolved
waku/waku_peer_exchange/protocol.nim Show resolved Hide resolved
@SionoiS
Copy link
Contributor

SionoiS commented May 2, 2024

What about the make file? Needs to be changed too no?

@Ivansete-status
Copy link
Collaborator Author

What about the make file? Needs to be changed too no?

If you refer to wakunode2 I think we can't change that easily and would require coordination with infra, and Dockerfile changes too.

Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, as I understand the general direction taken here. However, have asked a question on switching to ptr operations below.

proc getRunningNetConfig(app: App): AppResult[NetConfig] =
var conf = app.conf
let (tcpPort, websocketPort) = getPorts(app.node.switch.peerInfo.listenAddrs).valueOr:
proc getRunningNetConfig(waku: ptr Waku): Result[NetConfig, string] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard for me to see why in this module we have to switch to operations on ptrs. It causes some uncomfortable integrations into higher layer applications: startWaku(addr waku) is quite a bit less intuitive than waku.startWaku(). My assumption is that this is necessary for the thread management in the bindings? However, wouldn't it be better to wrap this into another ptr operation layer specifically (and only) for the bindings? May be missing some fundamental limitation here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment @jm-clius ! 🙌

I agree that it becomes more awkward to use with startWaku(addr waku)

Let me elaborate more on why this is needed:

  • We should make the startWaku proc async because the waitFor statement should be only invoked from the highest level to avoid deadlocks. i.e. the waitFor should only be invoked from within the wakunode2 code or highest level of libwaku.

    It is not correct to have a waitFor as we had before this PR. See:

    nwaku/waku/factory/app.nim

    Lines 177 to 179 in 853ec18

    proc startApp*(app: var App): AppResult[void] =
    let nodeRes = catch:
    (waitFor startNode(app.node, app.conf, app.dynamicBootstrapNodes))

  • The startWaku (formerly known as startApp) changes the state of the given parameter, waku, but we cannot longer pass a var parameter because the startWaku should be async (commented in the previous point) and it doesn't compile ( <<cannot be captured as it would violate memory safety>> .)

  • Given that we cannot use var, the only way to allow changing the state of a variable in an async proc, is by using ptr type, which is considered unsafe but in our case we can make sure that the passed Waku instance has longer lifetime than the call of startApp and it won't crash because of incorrect address being used inside the startApp.

Nevertheless, there might be a more elegant approach ( cc @arnetheduck :)

@Ivansete-status Ivansete-status merged commit 2463527 into master May 3, 2024
14 of 15 checks passed
@Ivansete-status Ivansete-status deleted the waku-code-shared-with-wakunode2-and-libwaku branch May 3, 2024 12:07
@Ivansete-status Ivansete-status changed the title refactor: big refactor to add waku component in libwaku instead of onlu waku node refactor: big refactor to add waku component in libwaku instead of only waku node May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants